-
Notifications
You must be signed in to change notification settings - Fork 792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aggregators refactoring #1326
Aggregators refactoring #1326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1326 +/- ##
==========================================
+ Coverage 93.04% 93.18% +0.14%
==========================================
Files 137 139 +2
Lines 3794 3932 +138
Branches 784 805 +21
==========================================
+ Hits 3530 3664 +134
- Misses 264 268 +4
|
I'm frustrated that we didn't come to a consensus at #1325 and there is going to be another place to discuss about the topic. In this PR, Point was generic parameterized // Previously
export interface Point {
value: Sum | LastValue | Distribution | Histogram;
timestamp: HrTime;
}
// In this PR
export interface Point<T> {
value: T;
timestamp: HrTime;
}
// usage
toPoint(): Point<AggregatorTypes>; Actually, this is no significant difference to exporters: they are still seeing an aggregator's Notably, in this PR, the assert.deepStrictEqual(
record1.aggregator.toPoint().value as Distribution,
{
count: 0,
last: 0,
max: -Infinity,
min: Infinity,
sum: 0,
}
); was removed to assert.deepStrictEqual(
record1.aggregator.toPoint().value,
{
count: 0,
last: 0,
max: -Infinity,
min: Infinity,
sum: 0,
}
); This is valid without the changes since it doesn't access the point value fields like record1.aggregator.toPoint().value.count // this is the problem that going to fail the type check. |
This is going nowhere, not sure why but you are not listening
|
It is important to point out that typescript does branch type narrowing. So with PR #1325, we can do: const metricRecord: MetricRecord = getRecordSomeWhere()
if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
const point = metricRecord.aggregator.toPoint()
// this point can be infered as Point<Histogram> without any manual type casting
// since aggregator can be determined as type of SumAggregatorType for the `metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM`
} But with this PR, we still can't do such nice things: const metricRecord: MetricRecord = getRecordSomeWhere()
if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
const point = metricRecord.aggregator.toPoint()
// this point is infered as Point< AggregatorTypes>, so type casting is still needed.
// accessing `point.value.count` needs a type casting
} Yes, we have to determine the aggregator type in exporters. But with #1325, we leverage typescript branch type narrowing so the only thing we need to do is to switching case on the |
And this is finally a solid example what exactly you are trying to achieve. Then you right |
closing this with regards to last comment |
@legendecas please add this explanation how you want to use metric records into your PR |
@obecny Sorry but this is ridiculous. You were outright disrespectful with both your original comments on #1325 and here, you didn't understood what @legendecas was trying to achieve and were insistent on the fact that he was "wrong" then when you get it, its "finally a solid example". Could you please next time refrain to say this when clearly there was a misunderstanding ? |
wow strong words, but I never meant to be disrespectful, I tried to show many times what I meant with a good will, looks like I was completely misunderstood. If you or @legendecas felt offended then apology it was never my intention. |
I am locking this conversation. Thank you @obecny for your apology, even though it was not your intention to offend. With a team this culturally diverse, there are bound to be misunderstandings and I hope we can use this as a learning opportunity. |
Refactors of aggregators so that
toPoint
doesn't need casting anymore